Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: jump to definition proof of concept #911

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

rtetley
Copy link
Collaborator

@rtetley rtetley commented Sep 20, 2024

WIP

cf coq/coq#19584

@gares
Copy link
Member

gares commented Sep 21, 2024

Changes to coq look good too.
I wonder if there is a way to circumvent jumping to dune's copy of the sources.
I mean, should the logic be in Coq or here?
@SkySkimmer added the print of the Loc to a user visible message, so I guess it should be in Coq

@SkySkimmer
Copy link
Contributor

@SkySkimmer added the print of the Loc to a user visible message, so I guess it should be in Coq

That logic is on the loc -> pp side though, vscoq only calls the qualid -> loc part from coq and does the rest on its own.

match get_context st o_pos with
| None -> log "No context found"; None
| Some _ ->
match parse_entry st loc (Pcoq.Prim.smart_global) pattern with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
match parse_entry st loc (Pcoq.Prim.smart_global) pattern with
match parse_entry st loc Pcoq.Prim.qualid pattern with

since you're only using the AN case of smart_global

@gares
Copy link
Member

gares commented Sep 21, 2024

It may still make sense to have the api on the coq side and let vscoq call it on the loc, no?

Anyway, good job!

@YaZko
Copy link

YaZko commented Oct 23, 2024

I wonder if there is a way to circumvent jumping to dune's copy of the sources.

I do not know if it's feasible, but as a user that would be fantastic if possible. Company-coq does indeed jumps to dune source, and it has always been a major pain.

@TheoWinterhalter
Copy link

I guess in the meantime something that works in most cases but not all is better than nothing?

Comment on lines +698 to +699
begin match Nametab.locate qid with
| ConstRef x -> begin match Declare.get_loc x with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will need updating as I moved get_loc and made it apply to more than constants
something like

Suggested change
begin match Nametab.locate qid with
| ConstRef x -> begin match Declare.get_loc x with
let extref = Nametab.locate_extended qid in
match Nametab.cci_src_loc extref with

should work

@SkySkimmer
Copy link
Contributor

Not sure how well this works with an editable document, I mean start with

(* some comment *)
Definition foo := 0.

Check foo.

evaluate the Definition, then edit the comment, then jump to def of the foo in Check

if editing the comment doesn't force re-evaluating the Definition then the loc will be wrong, but we don't want to re execute after editing comments (or whatever other edits we can detect as being trivial)

@gares
Copy link
Member

gares commented Nov 13, 2024

That is certainly true, although it seems a minor problem in practice.

I'm thinking loud here: if we manage to perform some pieces of the globalization phase at parse time, then parsing "Definition foo := ..." could tell us that we will generate Some.Logical.Path.foo and we could store the KN->loc map in a synterp summary (rather than an interp one). We could complement the map at interp time with interp-time-generated stuff we really want to be precise, and that part would need to be shifted/shiftable by the UI when it tries to reuse an old state on a new text. Exactly as we do for diagnostics.

In a way, since the map is about locs, it should be part of the parsing state, not the interp one.

@SkySkimmer
Copy link
Contributor

I'm thinking loud here: if we manage to perform some pieces of the globalization phase at parse time

That's impossible IMO

In a way, since the map is about locs, it should be part of the parsing state, not the interp one.

The map is about objects like globrefs which are part of the interp state.

@gares
Copy link
Member

gares commented Nov 13, 2024

That's impossible IMO

AFAIK synterp already computes the current modpath. From that one can compute the final KN of "foo". Am I missing something? (I know today the kn data type is generated using the env, etc... but in principle we know all the data to compute it even without the env)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants